Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Validate base branches in DefaultCommandRunner #1768

Merged
merged 1 commit into from
Aug 30, 2021

Conversation

minamijoyo
Copy link
Contributor

Fixes #1539

The branch matcher feature has been introduced in #1383, but the current implementation was broken and doesn't work at all (#1539). If my understanding is correct, there are two problems:

(1) The GlobalCfg has a default Repo instance which always matches any repositries and branches. Therefore the branch matcher never be functional.
(2) Validating base branches in DefaultPreWorkflowHooksCommandRunner.RunPreHooks() implicitly assumed that users customize pre_workflow_hooks, but the assumption isn't always true because it defaults to empty.

For (1), I added a new method MatchingRepo() to GlobalCfg to check BranchMatches() for a single Repo instance.
For (2), I moved validating branch to DefaultCommandRunner.validateCtxAndComment(). Since the method has already validated meta data of pull request, I think it's suitable place to check base branches, but please let me know if there is anywhere more suitable.

@minamijoyo minamijoyo requested a review from a team as a code owner August 23, 2021 09:13
Fixes runatlantis#1539

The branch matcher feature has been introduced in runatlantis#1383, but the current
implementation was broken and doesn't work at all (runatlantis#1539).

If my understanding is correct, there are two problems:

(1) The `GlobalCfg` has a default `Repo` instance which always matches
any repositries and branches. Therefore the branch matcher never be
functional.
(2) Validating base branches in
`DefaultPreWorkflowHooksCommandRunner.RunPreHooks()` implicitly assumed
that users customize `pre_workflow_hooks`, but the assumption isn't
always true because it defaults to empty.

For (1), I added a new method `MatchingRepo()` to `GlobalCfg` to check
`BranchMatches()` for a single `Repo` instance.

For (2), I moved validating branch to
`DefaultCommandRunner.validateCtxAndComment()`. Since the method has
already validated meta data of pull request, I think it's suitable place
to check base branches, but please let me know if there is anywhere more
suitable.
@minamijoyo minamijoyo force-pushed the fix-branch-restriction branch from 37f6b93 to 8037de4 Compare August 23, 2021 09:30
@nishkrishnan nishkrishnan merged commit 2600708 into runatlantis:master Aug 30, 2021
minamijoyo added a commit to minamijoyo/atlantis that referenced this pull request Sep 2, 2021
…nfig

Fixes runatlantis#1695

The branch matcher feature has been implemented in runatlantis#1383 and runatlantis#1768.
There is only an example for it, but not in the reference.

https://github.com/runatlantis/atlantis/pull/1383/files#diff-5dd8dd3b7c37191b78109efaaa1bb73184ff7a1690632d687fed7cd748847f5eR31-R34

Add missing the `branch` key in the reference for server side repo config.

I also add a warning for `mergeable` requirement to check the `branch`
setting because I think a typical branch protection rule only restricts
a default branch. We should let users know that someone can potentially
bypass it without the `branch` restriction in atlantis.
minamijoyo added a commit to minamijoyo/atlantis that referenced this pull request Sep 2, 2021
Fixes runatlantis#1695

The branch matcher feature has been implemented in runatlantis#1383 and runatlantis#1768.
There is only an example for it, but not in the reference.

https://github.com/runatlantis/atlantis/pull/1383/files#diff-5dd8dd3b7c37191b78109efaaa1bb73184ff7a1690632d687fed7cd748847f5eR31-R34

Add missing the `branch` key in the reference for server side repo config.

I also add a warning for `mergeable` requirement to check the `branch`
setting because I think a typical branch protection rule only restricts
a default branch. We should let users know that someone can potentially
bypass it without the `branch` restriction in atlantis.
nishkrishnan pushed a commit that referenced this pull request Sep 14, 2021
)

Fixes #1695

The branch matcher feature has been implemented in #1383 and #1768.
There is only an example for it, but not in the reference.

https://github.com/runatlantis/atlantis/pull/1383/files#diff-5dd8dd3b7c37191b78109efaaa1bb73184ff7a1690632d687fed7cd748847f5eR31-R34

Add missing the `branch` key in the reference for server side repo config.

I also add a warning for `mergeable` requirement to check the `branch`
setting because I think a typical branch protection rule only restricts
a default branch. We should let users know that someone can potentially
bypass it without the `branch` restriction in atlantis.
msarvar referenced this pull request in lyft/atlantis Sep 27, 2021
Fixes #1539

The branch matcher feature has been introduced in #1383, but the current
implementation was broken and doesn't work at all (#1539).

If my understanding is correct, there are two problems:

(1) The `GlobalCfg` has a default `Repo` instance which always matches
any repositries and branches. Therefore the branch matcher never be
functional.
(2) Validating base branches in
`DefaultPreWorkflowHooksCommandRunner.RunPreHooks()` implicitly assumed
that users customize `pre_workflow_hooks`, but the assumption isn't
always true because it defaults to empty.

For (1), I added a new method `MatchingRepo()` to `GlobalCfg` to check
`BranchMatches()` for a single `Repo` instance.

For (2), I moved validating branch to
`DefaultCommandRunner.validateCtxAndComment()`. Since the method has
already validated meta data of pull request, I think it's suitable place
to check base branches, but please let me know if there is anywhere more
suitable.
krrrr38 pushed a commit to krrrr38/atlantis that referenced this pull request Dec 16, 2022
Fixes runatlantis#1539

The branch matcher feature has been introduced in runatlantis#1383, but the current
implementation was broken and doesn't work at all (runatlantis#1539).

If my understanding is correct, there are two problems:

(1) The `GlobalCfg` has a default `Repo` instance which always matches
any repositries and branches. Therefore the branch matcher never be
functional.
(2) Validating base branches in
`DefaultPreWorkflowHooksCommandRunner.RunPreHooks()` implicitly assumed
that users customize `pre_workflow_hooks`, but the assumption isn't
always true because it defaults to empty.

For (1), I added a new method `MatchingRepo()` to `GlobalCfg` to check
`BranchMatches()` for a single `Repo` instance.

For (2), I moved validating branch to
`DefaultCommandRunner.validateCtxAndComment()`. Since the method has
already validated meta data of pull request, I think it's suitable place
to check base branches, but please let me know if there is anywhere more
suitable.
krrrr38 pushed a commit to krrrr38/atlantis that referenced this pull request Dec 16, 2022
…natlantis#1784)

Fixes runatlantis#1695

The branch matcher feature has been implemented in runatlantis#1383 and runatlantis#1768.
There is only an example for it, but not in the reference.

https://github.com/runatlantis/atlantis/pull/1383/files#diff-5dd8dd3b7c37191b78109efaaa1bb73184ff7a1690632d687fed7cd748847f5eR31-R34

Add missing the `branch` key in the reference for server side repo config.

I also add a warning for `mergeable` requirement to check the `branch`
setting because I think a typical branch protection rule only restricts
a default branch. We should let users know that someone can potentially
bypass it without the `branch` restriction in atlantis.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Branch matcher does not work in atlantis v0.17.0
2 participants